-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use EWMH for DisplayServerX11::_window_minimize_check()
implementation
#80036
Use EWMH for DisplayServerX11::_window_minimize_check()
implementation
#80036
Conversation
27dbb92
to
873187c
Compare
Seems to be broken on Fedora 38 XFCE (X11), but I don't if/how the behavior is different from the master branch. Whole new can of worms :) XFCE iconifies multiple windows from the same application into the same menu on the pager. On this PR and master, If you minimize a dialog, switch workspaces, and switch back, the dialog becomes visible again. Some other similar, but strange behavior may or may not be exclusive to the PR. This might not be a bad thing, since the dialog is modal and blocks interaction with the main editor window, but fiddling with the pager menu also causes the windows to hide and re-appear inexplicably. I don't know what exactly is going on here, but after a little more testing I have seen it in both master and the PR. Update: It looks like XFCE indicates the minimized state by greying out the window icon in the pager menu. This reflects the state when you minimize a window using either the pager or the window decorations. When you switch workspaces, you can see this go out of sync, the icon is still grey, but the dialog becomes visible again. Based on this visual cue, there is a disagreement between Godot and the window manager about this state (observed in master). |
Thanks for your first PR! I just enabled the CI/CD build, as it is blocked for first-time contributors by default. After the build's done, I will test it locally to see if that fixes the issue. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats! I'm approving your PR, it was pretty stable on my system. Be sure to update what @AThousandShips asked you to edit, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that versed in X11 stuff at the moment, but the changes sound fine!
I've always only heard about _NET_WM_STATE_HIDDEN
so hopefully it's handled better across implementations, if that makes sense. Apologies if I said something very dumb, take that with a huge grain of salt.
Unfortunately I couldn't personally see whether it actually fixes the issue in practice (I don't have a problematic software setup at hand) but it definitely seems to work fine as it did on sway/XWayland so no regression got introduced from my point of view.
As @adamscott said, don't forget to apply @AThousandShips's requested changes!
Windowing is always a mess, so thanks a lot for taking the time to debug this issue!
873187c
to
5666656
Compare
@AThousandShips @adamscott It is done :) |
That makes two of us :). Up until now, I have only ever dealt with toolkits like GTK+, wxWidgets, Qt, etc.
So during my spelunking, it seems like ICCCM was the original standard, published in 1994. It proposed a A lot of innovation happened in window managers in the following years, and this framework proved insufficient. Applications and window managers all started doing their own thing. Roughly 10 years later, in 2005 EWHM was standardized. Notably, this unified the extensions which were being employed by Gnome and KDE. It included the You know what? Maybe that's the issue. Godot was using a bunch of EWMH properties and this might be what was causing Xwayland to act strange. There isn't a 1:1 mapping between ICCCM states and EWHM states. It might be as if Godot is trying to speak two different languages to the window manager. I don't know this for sure though. It is just a hunch I developed while typing this. What is Xwayland supposed to do if there are both ICCCM properties and EWMH properties set and they are in conflict? Seems like "undefined behavior" territory. |
Might be, might be not. It's surely compositor dependent as, even with native X11, WMs have to set the right atoms themselves, so there's ought to be inconsistencies.
Yeah that feels like it. BTW I just found out that wlroots doesn't implement the whole slew of EWMH 1.3 goodies: https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/1496 That might be important to know if we want to use some of them. |
Good find. Wlroots is used by a significant number of Wayland compositors. |
@YuriSizov Flagged to merge! :) |
DisplayServerX11::_window_minimize_check()
implementation
For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info. |
Hi, I'm using ArchLinux with Gnome 44 under wayland and I don't really know if this is related to my issue BUT this PR makes Godot 4.1.1-stable usable without launching it with --single-window. Grabacion.de.pantalla.desde.2023-08-04.22-03-33.webm |
@akien-mga Should be fixed. I added the email address to the account and verified it. Also, I had no idea you could use markdown in titles lmao. @dannroda does it, like, stop shaking for good after dragging the window? Or only while dragging the window? When the window is being dragged, Godot receives a continuous stream of ConfigureNotify events. The method I modified in this PR apparently gets called every time one of these is processed. I have not observed this shaking behavior at all personally though. |
Yes, if dragged or maximized it just stop shaking until a new window is created/called |
Hi! |
Update on the testing front. I haven't really systematized a great way to check for regressions, but I have tried a few additional desktop environments / window managers.
XFCE remains the only DE I have observed windows un-minimize themselves when switching virtual desktops. |
Thanks! And congrats for your first merged Godot contribution 🎉 |
@PorkrollPosadist Thanks a ton! This issue was making me mad. |
Thanks @PorkrollPosadist! |
@PorkrollPosadist 🙇♂️ thank you for figuring this out, testing it, and getting it across the finish line. This issue is driving me bonkers in 4.2.dev3 and I'm very much looking forward to the release that includes your fix. |
Cherry-picked for 4.1.3. |
Proposed solution for issue #77333
After a lot of testing (some of which is included in the issue discussion) I observed multiple failure mechanisms which all ended up leading back to
DisplayServerX11::_window_minimize_check()
returning an unexpected result. Since EWMH features are used liberally throughout the rest of the DisplayServerX11 implementation, I figured I would use use EWMH to figure out the minimized state instead of ICCCM (the original implementation).In my limited testing so far, this solves the issue on Gnome/Wayland (Gentoo x86_64). I also tested it on an alternate machine (Fedora 38, Gnome/X11 x86_64). This patch is going to need some more testing to ensure it doesn't introduce any regressions (i.e. fix XWayland at the expense of breaking X11). At least a handful of X11 window managers and Wayland compositors should be tried. Also, it would be wise to test some Godot projects which make advanced use of it's multi-window GUI features to check some corner cases which are less likely to be exposed by point-and-click usage.